Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate modifying class parameters #811

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

franmomu
Copy link
Contributor

Ref #810 (comment)

@GromNaN do you mean something like this?

@franmomu franmomu added the Task label Dec 13, 2023
@franmomu franmomu added this to the 4.7.0 milestone Dec 13, 2023
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, I haven't reviewed the full implementation, but that's the idea. So that only people that changed a class parameter get the deprecation message.

@franmomu franmomu force-pushed the deprecate_parameters branch 3 times, most recently from 6e76da7 to ca11d91 Compare December 13, 2023 22:03
@GromNaN GromNaN requested a review from malarzm December 14, 2023 15:30
@GromNaN
Copy link
Member

GromNaN commented Dec 14, 2023

Just noted that we can deprecate parameters.

You can also deprecate container parameters in your extension to warn users about not using them anymore. This helps with the migration across major versions of an extension.
Deprecation is only possible when using PHP to configure the extension, not when using XML or YAML. Use the ContainerBuilder::deprecateParameter() method to provide the deprecation details:

   $containerBuilder->setParameter('acme_demo.database_user', $configs['db_user']);

   $containerBuilder->deprecateParameter(
       'acme_demo.database_user',
       'acme/database-package',
       '1.3',
       // optionally you can set a custom deprecation message
       '"acme_demo.database_user" is deprecated, you should configure database credentials with the "acme_demo.database_dsn" parameter instead.'
   );

@franmomu
Copy link
Contributor Author

Just noted that we can deprecate parameters.

You can also deprecate container parameters in your extension to warn users about not using them anymore. This helps with the migration across major versions of an extension.
Deprecation is only possible when using PHP to configure the extension, not when using XML or YAML. Use the ContainerBuilder::deprecateParameter() method to provide the deprecation details:

   $containerBuilder->setParameter('acme_demo.database_user', $configs['db_user']);

   $containerBuilder->deprecateParameter(
       'acme_demo.database_user',
       'acme/database-package',
       '1.3',
       // optionally you can set a custom deprecation message
       '"acme_demo.database_user" is deprecated, you should configure database credentials with the "acme_demo.database_dsn" parameter instead.'
   );

I didn't know about that, but apparently it's only available since 6.3: symfony/symfony#47719

@GromNaN
Copy link
Member

GromNaN commented Dec 14, 2023

Ok, so we can detect if method_exists($containerBuilder, 'deprecateParameter') and add the deprecation in that case. Developers who want to upgrade to DoctrineMongoDBBundle 5 should update to Symfony 6.4+ before.

That would be more simple than the conditional warning added in this PR.

@malarzm
Copy link
Member

malarzm commented Dec 14, 2023

ContainerBuilder::deprecateParameter() sounds awesome :) I'd suggest adding an additional note in UPGRADE file and we should be good.

Developers who want to upgrade to DoctrineMongoDBBundle 5 should update to Symfony 6.4+ before.

This is a good point 👍

@franmomu franmomu force-pushed the deprecate_parameters branch 2 times, most recently from 7530f80 to 2b87ebe Compare December 14, 2023 23:32
@franmomu
Copy link
Contributor Author

After trying this, not sure if we can use ContainerBuilder::deprecateParameter() because we are still using these parameters so apparently we cannot deprecate them with this method without triggering deprecations 😞

@malarzm
Copy link
Member

malarzm commented Dec 15, 2023

Weird, how Symfony suggests to use this deprecation then?

@GromNaN
Copy link
Member

GromNaN commented Dec 15, 2023

Weird, how Symfony suggests to use this deprecation then?

Maybe we can deprecate the parameters only in the next major version, once they are no longer used.

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM as this.

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oups, you should revert to the previous implementation.

@GromNaN GromNaN merged commit 57dc5c9 into doctrine:4.7.x Dec 19, 2023
13 checks passed
@GromNaN
Copy link
Member

GromNaN commented Dec 19, 2023

Thank you @franmomu

@franmomu franmomu deleted the deprecate_parameters branch December 20, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants